Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-73539] Disable YUI by default #10045

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Conversation

timja
Copy link
Member

@timja timja commented Dec 10, 2024

See JENKINS-73539.

This is the culmination of a lot of work over the years to slowly replace existing components that were built with YUI to alternatives.
A few examples:

A number of plugins and core changes have been prepared to make this possible.
See the tracking spreadsheet: https://docs.google.com/spreadsheets/d/1UjvtFmNmEdjMN5DUoFxJfBryA8q-E5_HwOzVKbVG9b0/edit?gid=969462704#gid=969462704

We aren't removing YUI at this time as @MarkEWaite requested that we wait till after the next LTS cutoff so-as vendors can change the default back if they still require YUI in their commercial products.

Testing done

Checked elements in the browser tools, by default no yui is there.

Disabled the flag in user settings and yui appears

Ran System.setProperty("jenkins.model.experimentalflags.RemoveYuiUserExperimentalFlag.defaultValue", "false") in the script console and yui appears without having to set a flag.

Clicked around and no issues found.
ATH is successful: jenkinsci/acceptance-test-harness#1861

PCT is successful: jenkinsci/bom#4084.

Proposed changelog entries

  • Disable the Yahoo! User Interface library by default

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@timja timja added web-ui The PR includes WebUI changes which may need special expertise major-rfe For changelog: Major enhancement. Will be highlighted on the top rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted and removed major-rfe For changelog: Major enhancement. Will be highlighted on the top labels Dec 10, 2024
@timja timja changed the title Disable YUI by default [JENKINS-73539] Disable YUI by default Dec 10, 2024
@mawinter69
Copy link
Contributor

Not sure if this is the right approach. The flag is a user specific thing, so when a plugin is installed that still requires YUI, it means that each and every user of that instance needs to enable it in their personal settings.
I would remove that experimental flag and instead introduce a system property that allows to define if YUI should be enabled or not. That way the admin of an instance can define at startup that YUI is still needed.

@timja timja mentioned this pull request Dec 10, 2024
6 tasks
@timja timja requested a review from mawinter69 December 10, 2024 21:49
@timja timja added ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-successful This PR has successfully passed the full plugin-compatibility-test suite labels Dec 11, 2024
@timja timja marked this pull request as ready for review December 11, 2024 09:31
@timja timja requested review from a team and MarkEWaite December 11, 2024 09:31
Copy link
Contributor

@janfaracik janfaracik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM <3 Been a long time coming for this one.

@timja
Copy link
Member Author

timja commented Dec 11, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 11, 2024
@mawinter69
Copy link
Contributor

good enough
Does it need the upgrade-guide-needed label?

@timja
Copy link
Member Author

timja commented Dec 11, 2024

good enough Does it need the upgrade-guide-needed label?

lets add just in case

@timja timja added the upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed label Dec 11, 2024
@timja timja merged commit 8c9caff into jenkinsci:master Dec 12, 2024
16 checks passed
@timja timja deleted the disable-yui branch December 12, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-successful This PR has successfully passed the full plugin-compatibility-test suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants